Skip to content

treewide: remove all cargoSha256 usage#323983

Merged
JohnRTitor merged 7 commits intoNixOS:stagingfrom
Aleksanaa:cargoSha256-change
Jul 4, 2024
Merged

treewide: remove all cargoSha256 usage#323983
JohnRTitor merged 7 commits intoNixOS:stagingfrom
Aleksanaa:cargoSha256-change

Conversation

@Aleksanaa
Copy link
Member

@Aleksanaa Aleksanaa commented Jul 2, 2024

Description of changes

First treewide (easy)

Treewide command:

find . -type f -name "*.nix" -print0 | xargs -0 sed -i 's/cargoSha256 = "sha256-/cargoHash = "sha256-/g'

Second treewide

Treewide bash script (which was executed in pkgs):

#!/usr/bin/env bash

process_line() {
    local filename=${1%:}
    if [[ $4 =~ \"(.*)\"\; ]]; then
      local sha256="${BASH_REMATCH[1]}"
    fi

    [[ -z $sha256 ]] && return 0

    local hash=$(nix hash to-sri --type sha256 $sha256)

    echo "Processing: $filename"
    echo "  $sha256 => $hash"

    sed -i "s|cargoSha256 = \"$sha256\"|cargoHash = \"$hash\"|" $filename
}

# split output by line
grep -r 'cargoSha256 = ' . | while IFS= read -r line; do
    # split them further by space
    read -r -a parts <<< "$line"
    process_line "${parts[@]}"
done

There are some exceptions, basically using a wrapped builder, or writing in let. I've manually fixed them. Would be better to check again.

buildRustPackage changes

I'm not really sure about this part.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 6.topic: vim Advanced text editor labels Jul 2, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 1001-2500 This PR causes many rebuilds on Darwin and should most likely target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Jul 2, 2024
@Aleksanaa
Copy link
Member Author

Logically speaking it should not cause such rebuilds (or actually it will)?

@Aleksanaa Aleksanaa marked this pull request as draft July 2, 2024 10:17
@JohnRTitor
Copy link
Member

Yeah, I am not sure if it's worth the rebuilding effort. Improvements like this can come atomically, like when updating that specific package.

@Aleksanaa
Copy link
Member Author

Aleksanaa commented Jul 2, 2024

Ah, I see. The hash here will not only be consumed by buildRustPackage, it also exists as an environment variable.

Improvements like this can come atomically, like when updating that specific package.

In theory we should deprecate it and avoid further usages altogether. We have already done that for golang's vendorHash (vendorSha256).

@h7x4
Copy link
Member

h7x4 commented Jul 2, 2024

Yeah, I am not sure if it's worth the rebuilding effort. Improvements like this can come atomically, like when updating that specific package.

I don't know about this. It feels like it's better to do a treewide into staging and let hydra take the hit once, rather than keep spending review time on it for random PRs. It will probably also make a higher percentage of new PRs use cargoHash, seeing how people often use another package as reference.

@Aleksanaa
Copy link
Member Author

Once we have all treewide changes done, we can let it throw a warning, and let cargoSha256 a history. I'm currently considering completing other changes at the same time.

@github-actions github-actions bot added the 6.topic: TeX Issues regarding texlive and TeX in general label Jul 3, 2024
@Aleksanaa Aleksanaa changed the title treewide: change cargoSha256 with SRI hash to cargoHash treewide: remove all cargoSha256 usage Jul 3, 2024
@Aleksanaa Aleksanaa force-pushed the cargoSha256-change branch 3 times, most recently from ea1c933 to bde993f Compare July 3, 2024 12:53
@Aleksanaa Aleksanaa changed the base branch from master to staging July 3, 2024 12:56
@Aleksanaa Aleksanaa marked this pull request as ready for review July 3, 2024 12:59
@Aleksanaa Aleksanaa force-pushed the cargoSha256-change branch from bde993f to c23dfb1 Compare July 3, 2024 13:00
@Aleksanaa Aleksanaa requested a review from Mic92 July 3, 2024 13:01
@github-actions github-actions bot added the 8.has: documentation This PR adds or changes documentation label Jul 3, 2024
@Aleksanaa Aleksanaa force-pushed the cargoSha256-change branch from 03b9e02 to 7cf5762 Compare July 3, 2024 13:16
@Mic92
Copy link
Member

Mic92 commented Jul 3, 2024

Yeah, I am not sure if it's worth the rebuilding effort. Improvements like this can come atomically, like when updating that specific package.

Most of the time it won't make a difference because something else in staging will have caused the rust toolchain to rebuild from source anyway.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes labels Jul 3, 2024
@github-actions github-actions bot added the 6.topic: cinnamon Desktop environment label Jul 3, 2024
@Aleksanaa
Copy link
Member Author

Oh the conflict is in staging, not master

@Aleksanaa Aleksanaa marked this pull request as draft July 3, 2024 13:31
@Aleksanaa Aleksanaa force-pushed the cargoSha256-change branch from 5522af8 to 2e3cd81 Compare July 3, 2024 13:41
Aleksanaa added 7 commits July 3, 2024 21:53
This is done with the following bash script:

```
#!/usr/bin/env bash
process_line() {
    local filename=${1%:}
    if [[ $4 =~ \"(.*)\"\; ]]; then
      local sha256="${BASH_REMATCH[1]}"
    fi
    [[ -z $sha256 ]] && return 0
    local hash=$(nix hash to-sri --type sha256 $sha256)
    echo "Processing: $filename"
    echo "  $sha256 => $hash"
    sed -i "s|cargoSha256 = \"$sha256\"|cargoHash = \"$hash\"|"
$filename
}

# split output by line
grep -r 'cargoSha256 = ' . | while IFS= read -r line; do
    # split them further by space
    read -r -a parts <<< "$line"
    process_line "${parts[@]}"
done

```
@Aleksanaa Aleksanaa force-pushed the cargoSha256-change branch from 2e3cd81 to f6ee8a0 Compare July 3, 2024 13:55
@Aleksanaa Aleksanaa marked this pull request as ready for review July 3, 2024 13:55
@github-actions github-actions bot removed 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: cinnamon Desktop environment labels Jul 3, 2024
@Aleksanaa Aleksanaa removed the request for review from mkg20001 July 3, 2024 13:56
@ofborg ofborg bot added 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches. and removed 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 1001-2500 This PR causes many rebuilds on Darwin and should most likely target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Jul 3, 2024
Copy link
Member

@JohnRTitor JohnRTitor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, passes my eyeball test

@JohnRTitor JohnRTitor merged commit 410d121 into NixOS:staging Jul 4, 2024
wolfgangwalther added a commit to wolfgangwalther/nixpkgs that referenced this pull request Jul 8, 2024
This is a mismatch after merging both of:
- NixOS#323983
- NixOS#322749
@doronbehar
Copy link
Contributor

Missed rust-analyzer, fix is at:

#326491

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 6.topic: TeX Issues regarding texlive and TeX in general 6.topic: vim Advanced text editor 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants